8304498: JShell does not switch to raw mode when there is no /bin/test #13100

lahodaj · 2023-03-20T12:10:04Z

If JShell is run on a system that does not have /bin/test (which is, apparently, possible for some systems, which only have /usr/bin/test), it won't switch the terminal into the raw mode, and the input will not work properly.

The proposed fix herein is to detect whether test existing in /usr/bin/test, and if yes, use that location. Use the existing /bin/test otherwise.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8304498: JShell does not switch to raw mode when there is no /bin/test

Reviewers

Sean Coffey (@coffeys - Reviewer)
Vicente Romero (@vicente-romero-oracle - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13100/head:pull/13100
$ git checkout pull/13100

Update a local copy of the PR:
$ git checkout pull/13100
$ git pull https://git.openjdk.org/jdk.git pull/13100/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13100

View PR using the GUI difftool:
$ git pr show -t 13100

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13100.diff

bridgekeeper · 2023-03-20T12:11:03Z

👋 Welcome back jlahoda! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-03-20T12:13:27Z

@lahodaj The following labels will be automatically applied to this pull request:

core-libs
kulla

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-03-20T12:17:18Z

Webrevs

01: Full - Incremental (f7631210)
00: Full (159ef1a0)

AlanBateman · 2023-03-20T13:26:17Z

As a general point, we probably need to change jline to not create sub-processes. If we needs to introspect the environment then it will need to do it in the current VM.

coffeys · 2023-03-22T14:39:25Z

src/jdk.internal.le/share/classes/jdk/internal/org/jline/utils/OSUtils.java

@@ -102,4 +103,13 @@ public class OSUtils {
 TEST_COMMAND = test;
 }

+ private static boolean isTestCommandValid(String command) {
+ try {
+ Process p = new ProcessBuilder(command, "-z", "").inheritIO().start();


is there a reason why you chose to spin up a process here ? Would a test for an executable file be sufficient ?

…ested.

coffeys

LGTM

the test call seems to be only used in one method where JLine attempts to test the tty fd value (AFAICT) - they're hacking into java.lang.ProcessBuilder$RedirectPipeImpl for that purpose.

wonder if there's anything else the JDK libs can offer.

https://github.com/openjdk/jdk/blob/master/src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/exec/ExecTerminalProvider.java#L120

openjdk · 2023-03-23T13:06:02Z

@lahodaj This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8304498: JShell does not switch to raw mode when there is no /bin/test

Reviewed-by: coffeys, vromero

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 96 new commits pushed to the master branch:

6b2f34f: 8304718: GetIntArrayElements should not be passed JNI_FALSE
138cdc9: 8304694: Runtime exception thrown when break stmt is missing
46b0602: 8304547: Remove checking of -Djava.compiler in src/jdk.jdi/share/classes/com/sun/tools/jdi/SunCommandLineLauncher.java
8d423f7: 8304723: Statically allocate global mutexes
2083088: 8304828: Lots of constant static data not declared static const in cpu/x86
0712adc: 7169951: SwingSet2 throws NullPointerException with Nimbus L&F
de1c12e: 8301012: [vectorapi]: Intrinsify CompressBitsV/ExpandBitsV and add the AArch64 SVE backend implementation
10fa7d1: 8304804: Remove develop flag G1VerifyCTCleanup
4acf20d: 8304809: Remove develop flag G1ExitOnExpansionFailure
a5ffa07: 7026262: HttpServer: improve handling of finished HTTP exchanges
... and 86 more: https://git.openjdk.org/jdk/compare/8d2ebf248e2884fbf138b603ae82f81bd0926cf3...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

vicente-romero-oracle

looks sensible

lahodaj · 2023-03-28T16:54:20Z

/integrate

openjdk · 2023-03-28T16:56:43Z

Going to push as commit fab2357.
Since your change was applied there have been 121 commits pushed to the master branch:

1fc218c: 8303912: Clean up JavadocTokenizer
c1f5ca1: 8303623: Compiler should disallow non-standard UTF-8 string encodings
695683b: 8304880: [PPC64] VerifyOops code in C1 doesn't work with ZGC
83ce65e: 8305004: add @SPEC tags to langtools modules
32ef452: 8304884: Update Bytecodes data to be mostly compile time constants
927e674: 8300977: Retire java.io.ExpiringCache
c90699e: 8304989: unnecessary dash in @param gives double-dash in docs
395a4ce: 8304591: (fs) UnixPath.stringValue need not be volatile
60640a2: 8300979: Lazily initialize (byte, char)arr in java.io.DataInputStream
cddaf68: 8304962: sun/net/www/http/KeepAliveCache/B5045306.java: java.lang.RuntimeException: Failed: Initial Keep Alive Connection is not being reused
... and 111 more: https://git.openjdk.org/jdk/compare/8d2ebf248e2884fbf138b603ae82f81bd0926cf3...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-03-28T16:57:02Z

@lahodaj Pushed as commit fab2357.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

turbanoff · 2023-03-28T18:33:46Z

@lahodaj was the issue reported to the upstream?

lahodaj · 2023-03-29T15:12:54Z

@lahodaj was the issue reported to the upstream?

I've just filled:
jline/jline3#839

8304498: JShell does not switch to raw mode when there is no /bin/test

159ef1a

openjdk bot added the rfr Pull request is ready for review label Mar 20, 2023

openjdk bot added core-libs core-libs-dev@openjdk.org kulla kulla-dev@openjdk.org labels Mar 20, 2023

coffeys reviewed Mar 22, 2023

View reviewed changes

Checking the executable flags instead of running the program, as sugg…

f763121

…ested.

coffeys approved these changes Mar 23, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 23, 2023

vicente-romero-oracle approved these changes Mar 27, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 28, 2023

openjdk bot closed this Mar 28, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8304498: JShell does not switch to raw mode when there is no /bin/test #13100

8304498: JShell does not switch to raw mode when there is no /bin/test #13100

lahodaj commented Mar 20, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 20, 2023

openjdk bot commented Mar 20, 2023

mlbridge bot commented Mar 20, 2023 •

edited

Loading

AlanBateman commented Mar 20, 2023

coffeys Mar 22, 2023

coffeys left a comment

openjdk bot commented Mar 23, 2023 •

edited

Loading

vicente-romero-oracle left a comment

lahodaj commented Mar 28, 2023

openjdk bot commented Mar 28, 2023

openjdk bot commented Mar 28, 2023

turbanoff commented Mar 28, 2023

lahodaj commented Mar 29, 2023

8304498: JShell does not switch to raw mode when there is no /bin/test #13100

8304498: JShell does not switch to raw mode when there is no /bin/test #13100

Conversation

lahodaj commented Mar 20, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 20, 2023

openjdk bot commented Mar 20, 2023

mlbridge bot commented Mar 20, 2023 • edited Loading

Webrevs

AlanBateman commented Mar 20, 2023

coffeys Mar 22, 2023

Choose a reason for hiding this comment

coffeys left a comment

Choose a reason for hiding this comment

openjdk bot commented Mar 23, 2023 • edited Loading

vicente-romero-oracle left a comment

Choose a reason for hiding this comment

lahodaj commented Mar 28, 2023

openjdk bot commented Mar 28, 2023

openjdk bot commented Mar 28, 2023

turbanoff commented Mar 28, 2023

lahodaj commented Mar 29, 2023

lahodaj commented Mar 20, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 20, 2023 •

edited

Loading

openjdk bot commented Mar 23, 2023 •

edited

Loading